Adding availableKeys to ParseObject.State #596

natario1 · 2017-03-11T17:22:49Z

Adds a availableKeys property to ParseObject.state that includes keys that can be safely accessed. Fixes IllegalStateExceptions thrown when using query.selectKeys() #595 so now there is consistency between complete and non-complete objects.

While it seems to work on my machine, I have no deep understanding of the library. Nothing should be broken though.
Exposing ParseObject.isDataAvailable(String key). Right now there are:
- ParseObject.has(String key): alias to containsKey(key)
- ParseObject.containsKey(String key): true if the server sent out data for this key
Neither of these can be used to know if ParseObject.get* methods can be called safely. In some cases has(key) returns false but get(key) can be called without throwing. In some others has(key) returns false and get(key) will throw.

This PR exposes isDataAvailable(String key) for partially fetched objects. When false, get* methods will throw. So with the PR:
- ParseObject.containsKey(String key): we have some data for this key from the server.
- ParseObject.isDataAvailable(): the object is complete, all getters can be called.
- ParseObject.isDataAvailable(String key): the object might not be complete, but you can call get(key) safely.
This makes sense to me.
Fixes No support for dotted notation in query.selectKeys() #597 adding support for nested selected keys. If there are some, nested objects will not be considered complete.

facebook-github-bot · 2017-03-11T17:52:09Z

@natario1 updated the pull request - view changes

rogerhu · 2017-03-11T19:22:07Z

Thanks. Do you know how the iOS version handles this issue?

On Sat, Mar 11, 2017, 9:22 AM Mattia ***@***.***> wrote: - Adds a safeKeys property to ParseObject.state that includes keys that can be safely accessed. Fixes #595 <#595> so now there is consistency between complete and non-complete objects. While it seems to work on my machine, I have no deep understanding of the library. Nothing should be broken though. - Exposing ParseObject.isDataAvailable(String key). Right now there are: ParseObject.has(String key): alias to containsKey(key) ParseObject.containsKey(String key): true if the server sent out data for this key Neither of these can be used to know if ParseObject.get* methods can be called safely. In some cases has(key) returns false but get(key) can be called without throwing. In some others has(key) returns false and get(key) will throw. This PR exposes isDataAvailable(String key) for partially fetched objects. When false, get* methods will throw. So with the PR: ParseObject.has(String key): we have some data for this key from the server. ParseObject.containsKey(String key): same. ParseObject.isDataAvailable(): the object is complete, all getters can be called. ParseObject.isDataAvailable(String key): the object might not be complete, but you can call get(key) safely. This makes sense to me. ------------------------------ You can view, comment on, or merge this pull request online at: #596 Commit Summary - Add safeKeys that can be safely accessed. Exposing isDataAvailable(key) File Changes - *M* Parse/src/main/java/com/parse/NetworkQueryController.java <https://github.com/ParsePlatform/Parse-SDK-Android/pull/596/files#diff-0> (2) - *M* Parse/src/main/java/com/parse/ParseObject.java <https://github.com/ParsePlatform/Parse-SDK-Android/pull/596/files#diff-1> (39) Patch Links: - https://github.com/ParsePlatform/Parse-SDK-Android/pull/596.patch - https://github.com/ParsePlatform/Parse-SDK-Android/pull/596.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#596>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAT8yWCWnGFMtnutVzSrsQT8kks6mniKks5rkthpgaJpZM4MaRF1> .

natario1 · 2017-03-11T19:47:43Z

@rogerhu I don't know. As far as I know the JS sdk simply does not throw, so you can call get with any key, and if not present, you get undefined.

facebook-github-bot · 2017-03-12T11:24:44Z

@natario1 updated the pull request - view changes

flovilmart · 2017-03-12T16:04:41Z

What's the behavior on the iOS SDK? Should remove the throwing altogether?

rogerhu · 2017-03-12T16:06:34Z

So iOS Parse has selectKeys() and findObjects() as part of PFQuery and PFQueryState

Is selectedKeys() the equivalent of safeKeys? (https://github.com/ParsePlatform/Parse-SDK-iOS-OSX/blob/7a820b75c6726808475af9ce705053ff0e8cbd11/Tests/Unit/QueryStateUnitTests.m)

facebook-github-bot · 2017-03-12T16:35:07Z

@natario1 updated the pull request - view changes

natario1 · 2017-03-12T16:35:20Z

@rogerhu we also have selectedKeys() as part of ParseQuery.State and that’s used to understand if the object should be considered ‘complete’. So if you select some keys all the objects will be incomplete. The issue is that when objects are incomplete and you call get* on a legit key, ParseObject will throw.

The PR adds safeKeys to ParseObject.State , meaning, ‘keys we don’t currently have in our data, but you should be able to call get() on without crashing the app’.

This is dirty and I would be OK with @flovilmart with simply removing the throwing.

rogerhu · 2017-03-12T17:30:37Z

Where do we need to remove IllegalStateException throwing? I think that makes more sense too to keep consistent with the other SDKs.

natario1 · 2017-03-12T18:00:01Z

I have found how iOS handles this issue, exactly here.

There’s a availableKeys set, that holds actual "keys received from the server" plus "keys selected on the query".

This is practically the same to this PR (except that here the safeKeys Set means "keys selected on the query but not received from the server"). So this looks OK to me. What do you think?

rogerhu · 2017-03-12T18:01:15Z

Should we call it availableKeys instead then?

rogerhu · 2017-03-12T18:04:46Z

Does it deal with dotted notation as well?

facebook-github-bot · 2017-03-12T18:14:56Z

@natario1 updated the pull request - view changes

natario1 · 2017-03-12T18:15:20Z

Yes, I can refactor to give the same meaning it has in the iOS SDK. It’s a bit risky though, because in that case we must ensure that the availableKeys set is always consistent with the serverData map.

I don’t fully understand that part (server data is held in two different maps both in ParseObject and in ParseObject.State...).

Yes, this implements dotted notation.

rogerhu · 2017-03-12T18:19:34Z

I'm more curious if there's anywhere in the iOS code that deals with dotted notation issues as well...

natario1 · 2017-03-12T18:31:58Z

@rogerhu it seems they don't, so they should have the #597 bug.

The relevant part is here: every decoded nested object is created as ‘complete’, which is wrong if some keys where selected.

The exact same line for the Android SDK is here and this fixes it.

facebook-github-bot · 2017-03-12T18:58:30Z

@natario1 updated the pull request - view changes

rogerhu · 2017-03-12T20:30:44Z

Parse/src/main/java/com/parse/ParseObject.java

+ JSONArray nestedKeys = new JSONArray();
+ for (int i = 0; i < selectedKeys.length(); i++) {
+ String nestedKey = selectedKeys.getString(i);
+ if (nestedKey.startsWith(key+".")) nestedKeys.put(nestedKey.substring(key.length()+1));


What happena if this is multiple nested keys? A.b.c.d.e?

At first (942-954) the current object will be given its own direct keys (without dots, e.g. b).
Later (958+) we extract the child object subkeys (e.g. b.c.d.e). That is passed to decoder.decode(), which will pass back the JSON object to this method. It will get ‘b’ as a safe key, and will pass ‘c.d.e’... and so on. It goes this way with nested iterations, until there are no nested ParseObjects.

Great thanks for the explanation

Do we have a test for this part?

I have added two in ParseDecoderTest, I will add another one for the a.b situation.

Thanks also the key + ”.” could be spaced out

rogerhu · 2017-03-12T20:48:22Z

Parse/src/test/java/com/parse/ParseObjectTest.java

@@ -237,7 +238,16 @@ public void testGetUnavailable() {
 ParseObject.State state = mock(ParseObject.State.class);
 when(state.className()).thenReturn("TestObject");
 when(state.isComplete()).thenReturn(false);
+ ParseObject object = ParseObject.from(state);
+ object.get("foo");


Is this just an assertion that no exception will be raised?

No, this checks that the exception is raised (see the @test annotation). Actually I have not changed this method, I have added testGetAvailableIfKeySafe() a few lines below, that tests safeKeys() .

rogerhu · 2017-03-12T21:47:14Z

Parse/src/main/java/com/parse/ParseObject.java

 * @param json
 * The object's data.
 * @param defaultClassName
 * The className of the object, if none is in the JSON.
 * @param isComplete
 * {@code true} if this is all of the data on the server for the object.
 * @param decoder
- * Delegate for knowing how to decode the values in the JSON.


Was this line deleted accidentally?

Yes, good catch.

facebook-github-bot · 2017-03-12T21:56:55Z

@natario1 updated the pull request - view changes

rogerhu · 2017-03-12T23:08:40Z

Parse/src/main/java/com/parse/NetworkQueryController.java

 for (int i = 0; i < results.length(); ++i) {
 JSONObject data = results.getJSONObject(i);
- T object = ParseObject.fromJSON(data, resultClassName, state.selectedKeys() == null);
+ if (isSubset) data.put(ParseObject.KEY_SELECTED_KEYS, selectedKeys);


Can you pass in selectedKeys into a method instead that accepts selectedKeys() instead?

Similar to:

https://github.com/ParsePlatform/Parse-SDK-iOS-OSX/blob/master/Parse/PFObject.m#L772-L774

It seems weird that the NetworkQueryController has access to this private key..

@rogerhu I get your point, I tried working directly with a Set, but for that we would need to change the signatures of ParseObject.fromJSON, ParseObject.mergeFromServer, and even worse ParseDecoder.decode, these are called from anywhere and an extra set of "selected keys" rarely makes sense.
The key is also used by ParseDecoder to get keys from the JSON...

Maybe we could add two static methods to ParseObject, void insertSelectedKeys(JSONObject data, Set<String>) , used by NetworkQueryController, and boolean hasSelectedKeys(JSONObject data) , used by ParseDecoder. What do you think?

Or I’ll just add another ParseObject.fromJSON that deals with this.

rogerhu · 2017-03-12T23:17:19Z

Parse/src/main/java/com/parse/ParseDecoder.java

@@ -122,7 +122,7 @@ public Object decode(Object object) {
 }

 if (typeString.equals("Object")) {
- return ParseObject.fromJSON(jsonObject, null, true, this);
+ return ParseObject.fromJSON(jsonObject, null, !jsonObject.has(ParseObject.KEY_SELECTED_KEYS), this);


it seems to me that fromJSON() should figure out whether things are complete instead of here right?

Just trying to get KEY_SELECTED_KEYS out of being a public static variable..

rogerhu · 2017-03-12T23:42:59Z

Thanks for working on this bug/issue..quite complicated to fix and appreciate the effort. Most of my feedback are minor issues in general.

facebook-github-bot · 2017-03-13T00:03:07Z

@natario1 updated the pull request - view changes

rogerhu · 2017-03-12T23:38:52Z

Parse/src/main/java/com/parse/ParseObject.java

@@ -98,6 +99,7 @@ private static ParseObjectSubclassingController getSubclassingController() {
 private long createdAt = -1;
 private long updatedAt = -1;
 private boolean isComplete;
+ private Set<String> safeKeys = new HashSet<>();
 /* package */ Map<String, Object> serverData = new HashMap<>();


I'd put a comment here that these are the keys returned from the server but the object may not have defined...Or whatever best way to phrase to explain the intention. It took me all day to get my ahead around this...

Haha yes. Actually the server does not return these, we have them because of query.selectKeys(), but you got the point. Annoying bug.

rogerhu · 2017-03-12T23:40:50Z

Parse/src/main/java/com/parse/ParseObject.java

+ if (nestedKeys.length() > 0) {
+ ((JSONObject) value).put(KEY_SELECTED_KEYS, nestedKeys);
+ }
+ }
 Object decodedObject = decoder.decode(value);


Instead of having to pass selected keys as metadata does it help to modify the signature in any way?

Well we need a fromJSON signature that does not take selectedKeys() .. because ParseDecoder will just have a JSON and is not aware of the selected keys bundled in the JSON (I made KEY_SELECTED_KEYS private as you said), see.

From this point on, bundling the keys in the JSON allows us to not change all the other signatures, and abstract this internal stuff from ParseDecoder. It is also used in Local data store encoding/decoding, see ParseObject.toRest() / fromREST()

Gotcha forgot about local store too

facebook-github-bot · 2017-03-13T01:51:14Z

@natario1 updated the pull request - view changes

rogerhu · 2017-03-13T01:54:47Z

Lgtm thanks

flovilmart · 2017-03-13T02:17:03Z

Can we rename those to availableKeys?

rogerhu · 2017-03-13T03:00:59Z

@natario1 how easy would it be to change the construct to be availableKeys? The main thing I think is this function here: https://github.com/natario1/Parse-SDK-Android/blob/fcf24e3af94f0321d80d399c418a325e7e3da90f/Parse/src/main/java/com/parse/ParseObject.java#L168-L174

Mostly I think safeKeys() is implemented like this:

if (!serverData.containsKey(key)) safeKeys.add(key);

facebook-github-bot · 2017-03-13T10:30:33Z

@natario1 updated the pull request - view changes

natario1 · 2017-03-13T10:47:13Z

Ok guys, reverted that logic and renamed to availableKeys() .

Since this has become something serious, the next design question would be if we should add the set also to ParseObject (not only ParseObject.State). As far as I understand the ParseObject.State is synced with the server, while ParseObject members do also trace dirty changes.

So, for example, you have the ParseObject.State.serverData map and the ParseObject.estimatedData map. The former comes from the server. The latter comes from the server as well, but is modified by put operations. It appears they are synced together after save operations.

So, now that we have ParseObject.State.availableKeys, should we also add ParseObject.estimatedKeys?

Pros: would allow to change this line from

  public boolean isDataAvailable(String key) {
    synchronized (mutex) {
      // Fallback to estimatedData to include dirty changes.
     return isDataAvailable() || state.availableKeys().contains(key) || estimatedData.containsKey(key);
    }
  }

to a much cleaner

  public boolean isDataAvailable(String key) {
    synchronized (mutex) {
     return isDataAvailable() || estimatedKeys.contains(key);
    }
  }

Pros: consistency with estimatedData and with the iOS SDK implementation of isDataAvailable
Cons: adding a useless Set for every ParseObject (it will differ from estimatedData.keySet() only when the object comes from a Query with selected keys...) to do what we are already doing.
Cons: must be 100% sure that estimatedKeys follows operation onto estimatedData or we will throw when we shouldn't. Changing the implementation of an important method (isDataAvailable(key)) and I don’t think I have enough understanding of the class.

What do you think @rogerhu @flovilmart ? I think it can be part of a future PR ...

natario1 · 2017-03-13T10:52:47Z

Just to clarify, the current implementation (with this PR) of isDataAvailable(key) does follow dirty changes. This works. We just have to check estimated data.

flovilmart · 2017-03-13T11:21:23Z

If it works, don't change the internal yet :)

rogerhu · 2017-03-13T12:19:51Z

Good enough for now. :). Thanks!

natario1 · 2017-03-15T19:20:33Z

When are you planning to release next version @rogerhu ?

flovilmart · 2017-03-15T19:21:57Z

Snapshots are released with each merge. That should be good to test no?

rogerhu · 2017-03-15T20:11:06Z

Working also on Bintray access so I can deploy the v14.0.0 version currently on snapshot too. :)

natario1 · 2017-03-15T20:58:00Z

Nice, no hurry. By the way, we are pushing 1.14.1 to Sonatype, is that a typo? (should be 1.14.0)

rogerhu · 2017-03-15T21:00:05Z

Yeah my fail. :) It's 1.14.1-SNAPSHOT (I'll fix it to 1.14.0)

rogerhu · 2017-03-20T08:06:04Z

Parse/src/main/java/com/parse/NetworkQueryController.java

@@ -136,7 +136,7 @@ public Integer then(Task<JSONObject> task) throws Exception {
 }
 for (int i = 0; i < results.length(); ++i) {
 JSONObject data = results.getJSONObject(i);
- T object = ParseObject.fromJSON(data, resultClassName, state.selectedKeys() == null);
+ T object = ParseObject.fromJSON(data, resultClassName, ParseDecoder.get(), state.selectedKeys());


@natario1 do you remember why we need to pass in ParseDecoder.get() here?

@rogerhu it’s just an instance of ParseDecoder.

I had to change the signatures of ParseObject.fromJSON and this seemed legit. ParseDecoder was present in the other signature (used here by ParseDecoder itself, passing this), so I added it here as well for consistency.

Since ParseDecoder is a singleton we could just drop it from both signatures, but I guess original authors wanted to keep the chance of passing a different decoder. I don’t know.

Add safeKeys that can be safely accessed. Exposing isDataAvailable(key)

301ec93

facebook-github-bot added CLA Signed labels Mar 11, 2017

Add tests

2827522

Support for LocalDataStore; fixed some bugs

b71193b

natario1 mentioned this pull request Mar 12, 2017

No support for dotted notation in query.selectKeys() #597

Closed

Support for dot notation in selected keys

90877ae

Addressing nested keys without parent key

f009d34

Adding extra tests

bc1c1fa

rogerhu reviewed Mar 12, 2017

View reviewed changes

New test, fixed a bug

7d7cc45

rogerhu reviewed Mar 12, 2017

View reviewed changes

New signature for fromJSON

6d2ccde

rogerhu reviewed Mar 13, 2017

View reviewed changes

Added some comments

fcf24e3

Refactored safeKeys() into availableKeys()

b144ca0

natario1 changed the title ~~Adding safeKeys that can be safely accessed to ParseObject.State~~ Adding availableKeys to ParseObject.State Mar 13, 2017

rogerhu merged commit ddf8de4 into parse-community:master Mar 13, 2017

rogerhu reviewed Mar 20, 2017

View reviewed changes

natario1 deleted the selectkeys branch May 1, 2017 10:25

Adding availableKeys to ParseObject.State #596

Adding availableKeys to ParseObject.State #596

Conversation

natario1 commented Mar 11, 2017 • edited Loading

facebook-github-bot commented Mar 11, 2017

rogerhu commented Mar 11, 2017 via email

natario1 commented Mar 11, 2017

facebook-github-bot commented Mar 12, 2017

flovilmart commented Mar 12, 2017

rogerhu commented Mar 12, 2017 • edited Loading

facebook-github-bot commented Mar 12, 2017

natario1 commented Mar 12, 2017

rogerhu commented Mar 12, 2017

natario1 commented Mar 12, 2017

rogerhu commented Mar 12, 2017

rogerhu commented Mar 12, 2017

facebook-github-bot commented Mar 12, 2017

natario1 commented Mar 12, 2017

rogerhu commented Mar 12, 2017

natario1 commented Mar 12, 2017

facebook-github-bot commented Mar 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 12, 2017

rogerhu Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerhu commented Mar 12, 2017

facebook-github-bot commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 13, 2017

rogerhu commented Mar 13, 2017

flovilmart commented Mar 13, 2017

rogerhu commented Mar 13, 2017 • edited Loading

facebook-github-bot commented Mar 13, 2017

natario1 commented Mar 13, 2017 • edited Loading

natario1 commented Mar 13, 2017

flovilmart commented Mar 13, 2017

rogerhu commented Mar 13, 2017

natario1 commented Mar 15, 2017

flovilmart commented Mar 15, 2017

rogerhu commented Mar 15, 2017

natario1 commented Mar 15, 2017

rogerhu commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natario1 commented Mar 11, 2017 •

edited

Loading

rogerhu commented Mar 12, 2017 •

edited

Loading

rogerhu Mar 12, 2017 •

edited

Loading

rogerhu commented Mar 13, 2017 •

edited

Loading

natario1 commented Mar 13, 2017 •

edited

Loading